Bech32 orderbook #656

decentclock · 2020-08-04T14:52:22Z

Aims to create an orderbook for native segwit v0 addresses. See #632 for more discussion.

Notes on 9242200:

With this update, the maker passes its public key and signature ( no more third field scriptCode ), and the taker figures out how to construct the input correctly by looking at the output it is spending.
It seems to me that in the nested segwit case, both the SCRIPT_VERIFY_P2SH and the SCRIPT_VERIFY_WITNESS flags to VerifyScript need to be set. I've made the correction just in verify_tx_input but let me know if this is wrong.

Note on 0ae3c9e: For now, I have left the defaults for the maker's fake offer and yg-privacyenhanced.py on nested segwit.

For testing, I am at the point where the test suite passes. I have not yet tried to run the various scripts with the new offer types, but I thought I'd open this to give an update on where the code is right now. Let me know what additional testing we can do before merging into master.

EDIT: I also still need to do a write-up on the migration process.

RiccardoMasutti

Shouldn't we remove this part? 😊
Good job jules!

jmclient/jmclient/configure.py

MaxHillebrand

that's fantastic, thanks!

good choice to name it for example sw0absoffer, this allows easy upgrade to future segwit version, like taproot.

decentclock · 2020-08-04T18:16:39Z

good choice to name it for example sw0absoffer, this allows easy upgrade to future segwit version, like taproot.

Exactly what I had in mind!

decentclock · 2020-08-04T20:43:44Z

It seems to me that in the nested segwit case, both the SCRIPT_VERIFY_P2SH and the SCRIPT_VERIFY_WITNESS flags to VerifyScript need to be set. I've made the correction just in verify_tx_input but let me know if this is wrong.

I've taken a closer look at this, and I am now quite confident that I am correct here. 78bc162 is a small commit to correct this elsewhere in the codebase.

AdamISZ · 2020-08-05T12:03:41Z

Looking good so far, although I'm only kind of half way through reviewing. Thanks for diving in, it will be great to have another person with all-around knowledge of how this works :)

good choice to name it for example sw0absoffer, this allows easy upgrade to future segwit version, like taproot.

For sure, I agree with this choice. Note that of course any name would work just as well to do a "soft fork" style upgrade, just as we did previously (absoffer -> swabsoffer). And indeed we don't need to constantly lengthen these names :)

Testing with ygrunner, did successful direct send and also coinjoin with counterparties on regtest. Things seem to be working :) ***

Re: the script verify flags, that indeed looks like a necessary correction (in case it confuses anyone, not having all the right script verify flags has not been a cause for any failure in transactions up to now, because an incomplete verification doesn't prevent transactions going through, and there is of course no danger with the functionality as is, because if we count as 'valid' a tx that isn't valid, it just results in a failed transaction push onto the network; not dangerous with coinjoin). I wonder if @dgpv could explain the necessity for P2SH as well as WITNESS for WITNESS-only case, as I read the explanation and couldn't understand it.

*** There is a detail about running manual coinjoins: open_test_wallet_maybe uses a test_wallet_cls argument that is defaulted to SegwitLegacyWallet, change that to SegwitWallet if you use the special test wallet type (using hex seed) (if you don't know what I'm talking about here, please do figure it out at some point, I kind of desperately need people to use this testing infrastructure, somehow I can never convince other people to do it, so it's always me that has to do the actual sanity tests of whether one can do coinjoins with full flow through IRC on regtest).

dgpv · 2020-08-05T12:58:16Z

if @dgpv could explain the necessity for P2SH as well as WITNESS for WITNESS-only case, as I read the explanation and couldn't understand it.

I'm afraid that I would not be able to explain it either without taking the effort to dig into Bitcoin Core source and commit history. The code in python-bitcointx just mirrors the code in Bitcoin Core (https://github.com/bitcoin/bitcoin/blob/bf0dc356ac4a2bdeda1908af021dea2de0dfb35a/src/script/interpreter.cpp#L1646-L1654). Core developers who was involved with BIP141 code might know better. It seems to me that this requirement is an artifact of how Core code is structured, and since python-bitcointx aims to mirror this code quite close, the same requirement is present.

AdamISZ · 2020-08-05T13:18:23Z

jmbitcoin/jmbitcoin/secp256k1_transaction.py

    flags = set([SCRIPT_VERIFY_STRICTENC])
+    # np2wkh and p2wkh cases each require both flags
+    # See bitcointx/core/scripteval.py:1255


Link to a commit in case line numbers shift.

AdamISZ · 2020-08-05T13:20:40Z

jmclient/jmclient/configure.py

-# currently supported in Joinmarket coinjoins. Only set to "true"
-# if specifically advised to do so.
-native = false
+# Use native segwit (bech32) wallet.


Add a line to the comment to explain what happens if you set it to False (perhaps something like "if you set this to 'false', p2sh wrapped segwit (p2sh-p2wpkh) will be used when generating a new wallet; note that the default Joinmarket pit does not support this, and also note that pre-existing wallets do not change their type." ... well it's tricky to cover every detail, but these points seem like the most obvious ones to note.

note that the default Joinmarket pit does not support this

Making sure we are on the same page, do you mean that the default joinmarket pit does not support native segwit?

@AdamISZ friendly ping!

AdamISZ · 2020-08-05T13:23:25Z

jmclient/jmclient/maker.py

@@ -171,8 +171,7 @@ def on_tx_received(self, nick, tx_from_taker, offerinfo):
                # unchanged as (sig, pub).
                try:
                    sig, pub = [a for a in iter(tx.wit.vtxinwit[index].scriptWitness)]
-                    scriptCode = btc.pubkey_to_p2wpkh_script(pub)
-                    sigmsg = btc.CScript([sig]) + btc.CScript(pub) + scriptCode
+                    sigmsg = btc.CScript([sig]) + btc.CScript(pub)


Just stating the relatively obvious here: this means new code cannot do Joinmarket coinjoins with p2sh. I think it's the right decision, but in principle, we could also do a check on the wallet type and then change the sig msg to be one of these two syntaxes.
The reason I think it's the right choice is (a) keeping logic simpler and (b) if we have an upgrade like this, we do want to strongly encourage people to shift over. There are counterarguments though, I don't know if anyone wants to make that.

~~Given that decision though, this is the right code change, of course.~~

This was wrong, see Taker.on_sig() changes as to why.

In any case I plan to continue running P2SH yg for some time (as I previously did with P2PKH), instead of sweeping everything to a new native segwit bech32 wallet, but maybe it's ok to continue using v0.6.x for that. Downside - unability to do P2SH BIP78 payjoins from such wallet in case that is what receiving side is supporting.

Just stating the relatively obvious here: this means new code cannot do Joinmarket coinjoins with p2sh.

@AdamISZ I'm assuming that you mean that p2sh is nested p2wkh. It seems to me that those changes, in conjunction with those on the taker side, should still support p2sh. What I've done is I've brought the creation of scriptCode entirely on the taker side. These changes depend entirely on the assumption that p2sh is np2wkh as far as coinjoins are concerned. I'd encourage people to take a look at the taker.py changes to understand my reasoning here.

Downside - unability to do P2SH BIP78 payjoins from such wallet in case that is what receiving side is supporting.

@kristapsk If my reasoning is correct above, then I think P2SH BIP78 payjoins will still be possible with these changes?

I think it's the right decision, but in principle, we could also do a check on the wallet type and then change the sig msg to be one of these two syntaxes.

I actually think this would improve the code here and make it cleaner! I'm thinking of a simple if statement conditioned on the wallet type to set the correct syntax for sigmsg. This assumes that all the inputs from a single maker will be of the same type. If people like this, I'll make this change.

Downside - unability to do P2SH BIP78 payjoins from such wallet in case that is what receiving side is supporting.

Well, first, I think we should release 0.7.0 without this PR and second you could still do BIP78 payjoins with this PR included, just as today you can do BIP78 payjoins with native wallets.

Just stating the relatively obvious here: this means new code cannot do Joinmarket coinjoins with p2sh.

@AdamISZ I'm assuming that you mean that p2sh is nested p2wkh. It seems to me that those changes, in conjunction with those on the taker side, should still support p2sh. What I've done is I've brought the creation of scriptCode entirely on the taker side. These changes depend entirely on the assumption that p2sh is np2wkh as far as coinjoins are concerned. I'd encourage people to take a look at the taker.py changes to understand my reasoning here.

I wrote this comment before reading how you had changed Taker.on_sig() (see my extended comment on that below). It was not correct, so you can ignore this comment. (uh second edit:

so this is the maker code - if this maker wanted to do p2sh coinjoins it would want to support pre-"soft fork" takers, which will only accept three arguments. So yes it is probably better to flag on the wallet type).

I actually think this would improve the code here and make it cleaner! I'm thinking of a simple if statement conditioned on the wallet type to set the correct syntax for sigmsg. This assumes that all the inputs from a single maker will be of the same type. If people like this, I'll make this change.

So as per above, I agree this is probably the best. It doesn't hurt to add this functionality, even if we are defaulting new makers to bech32 (after this PR).

you could still do BIP78 payjoins with this PR included

My comment was about v0.6.x, where BIP78 payjoin support is not included.

AdamISZ · 2020-08-05T13:29:50Z

There is a problem here:

joinmarket-clientserver/scripts/sendpayment.py

Line 165 in 78bc162

fee_per_cp_guess = estimate_tx_fee(2, 2, txtype="p2sh-p2wpkh")

- it isn't a problem/error in this PR, but only now does it become an issue: if you see the other calls to estimate_tx_fee in the main code (not testing), you'll see that the calls specify tx_type = wallet_service.get_tx_type() to figure out an estimate depending on scriptPubKey type, so we need to make that happen here too (to be clear, the usage there in sendpayment is very much a "first near-total guess" but all the same).

Edit: it's also in tumbler.py, same thing.

jmclient/jmclient/taker.py

AdamISZ · 2020-08-05T14:00:34Z

jmclient/jmclient/taker.py

            elif len(sig_deserialized) == 3:
-                ver_sig, ver_pub, scriptCode =  sig_deserialized
+                ver_sig, ver_pub, trash = sig_deserialized


This is interesting. First, it's traditional to use _ for unused return values, like a, b, _ = func_that_returns_a_tuple() ... I suspect for linting reasons it's preferable, though not important.

That we formerly had that third item is interesting - it was mostly being used as a flag - thus the maker is clearly identifying he is providing a wrapped segwit signature - but secondarily as a sanity check on the redeemScript. But of course it technically wasn't necessary for that secondary reason because the pubkey is provided and you could reconstruct it from that; even though that represents an assumption, it's a fine one to make (anything else is unrecognized, which is already true for anything unexpected).

Here you've used the python-bitcointx functions to make the processing cleaner - take the sig and pub data and check the type of the scriptPubKey, then process it based on the assumption that it is one of those three types (so as before it assumes that any p2sh is p2sh-p2wpkh and fails otherwise.

There is an argument of course (a good one really) for generalising further so that we can accept any old inputs. It complicates the bot messaging protocol but we probably should have done it from day one, it's just our bitcoin backend library wasn't powerful enough (or safe enough I guess); we can have the bots just send full blown PSBTs (modulo the bandwidth, ouch), or just stripped down: scriptSig and scriptPubKey in bitcoin serialization. But that's for another day.

I think what you have in this function is good.

AdamISZ · 2020-08-05T15:58:16Z

if @dgpv could explain the necessity for P2SH as well as WITNESS for WITNESS-only case, as I read the explanation and couldn't understand it.

I'm afraid that I would not be able to explain it either without taking the effort to dig into Bitcoin Core source and commit history. The code in python-bitcointx just mirrors the code in Bitcoin Core (https://github.com/bitcoin/bitcoin/blob/bf0dc356ac4a2bdeda1908af021dea2de0dfb35a/src/script/interpreter.cpp#L1646-L1654). Core developers who was involved with BIP141 code might know better. It seems to me that this requirement is an artifact of how Core code is structured, and since python-bitcointx aims to mirror this code quite close, the same requirement is present.

Yes thanks that makes sense, appreciate the explanation. Since I was curious I pinged #bitcoin on IRC, harding gave an answer:

waxwing: the code block requires that P2SH be enabled in order for segwit to be enabled in order to check to see if a transaction includes witness data even though none of its inputs are either native segwit or P2SH segwit. I think the comment is just misphrased, it should probably say something like, "we can't correctly check for unexpected witness data".

Probably requires a bit more reading around Core's script validation code to understand fully.

AdamISZ · 2020-08-05T16:00:34Z

EDIT: I also still need to do a write-up on the migration process.

Yeah just an .md file in docs and the rest will go in release notes. This could be a good starting point (and sidenote, we should already have removed that, it's way old).

AdamISZ · 2020-08-05T16:23:14Z

Just some commentary on something most people might not think of:
PoDLE commitments should be fine unaltered, with the following caveat:

PoDLEs are hashes of the shifted pubkey corresponding to the pubkey which forms the basis of the scriptPubKey of a utxo which the taker is referring to and signing against, effectively, with the podle. The assumption the maker makes is that the type of scriptpubkey offered is the same as his own wallet, see:

joinmarket-clientserver/jmclient/jmclient/maker.py

Lines 91 to 93 in fbf6fb0

    
           if not self.wallet_service.pubkey_has_script( 
        
                   cr_dict['P'], res[0]['script']): 
        
               raise EngineError()

- this is what ties the key to the utxo.

So clearly this is fine in general; in almost all cases we generate podles from utxos actually in the wallet concerned, so they have to be the right type for that coinjoin. The only exception is "external" commitments (podles) which can be stored in a file as explained here (which is linked under 'funding the wallet' in the usage guide). I'm not sure anything can/should realistically be done to address this, since this feature appears to be largely unused. Perhaps we just drop a note somewhere (and at the same time move that commitments explainer page into docs/).

decentclock · 2020-08-08T15:32:36Z

This should fix all the issues / suggestions raised so far. I am confident we can deploy some testnet instances of this PR for testing. On regtest, the only thing of note I haven't been able to test thus far is the tumbler.py script. I'm having trouble creating an orderbook with enough liquidity with ygrunner.py.

I'll add a documentation commit for the migration process soon.

EDIT: There is a small detail I need to update please wait before reviewing.

decentclock · 2020-08-08T19:12:58Z

Ok now I think this is ready. Bear in mind that if you run the test suite with native=false, you will fail some tests. Ideally we would like all of them to pass as well, but this is beyond the scope of the PR.

I now have been able to run tumbler.py with ygrunner.py successfully.

AdamISZ · 2020-08-16T20:23:26Z

Problem in ob-watcher cropping up in testing:
This line:

joinmarket-clientserver/scripts/obwatch/ob-watcher.py

Line 44 in cdf2d9c

swoffers = list(filter(lambda x: x[0:2] == 'sw', offername_list))

is picking up 4 rather than 2 different order types, which is causing a database query to throw an exception (see the SQL in get_counterparty_count().

For now a simple fix to stop the error: change to

swoffers = list(filter(lambda x: x[0:3] == 'sw0', offername_list))

But I would suggest a fuller fix is to remove entirely the non-segwit offerbook feature and replace it with 'legacy segwit', which will mean several changes to the ob-watcher and its html. To be clear I don't think this is a blocking issue at all.

decentclock · 2020-08-19T17:33:21Z

3d6e325 should fix this. That being said I didn't take the time yet to faithfully reproduce the error by creating multiple types of offers at the same time with ygrunner, so I appreciate any cross checks.

Let me know if you'd like me to squash that latest commit.

EDIT: The big picture: The toggle non-segwit button is now a Rotate orderbooks button, which rotates the three types of orderbooks: legacy, nested and native segwit.

kristapsk · 2020-08-19T21:35:17Z

I think pre-segwit legacy orderbook could be dropped from ob-watcher entirely, it's always empty last few times I checked, don't think anybody still uses it. Also, it's not supported outside ob-watcher by JM-CS.

decentclock · 2020-08-19T22:45:42Z

I think pre-segwit legacy orderbook could be dropped from ob-watcher entirely, it's always empty last few times I checked, don't think anybody still uses it. Also, it's not supported outside ob-watcher by JM-CS.

@AdamISZ Thoughts on this? I think I agree with Kristaps here...

decentclock · 2020-09-04T10:51:32Z

@AdamISZ @kristapsk I went ahead and removed the pre-segwit legacy orderbook as requested.

chris-belcher · 2020-09-04T11:46:10Z

Bear in mind that if you run the test suite with native=false, you will fail some tests. Ideally we would like all of them to pass as well, but this is beyond the scope of the PR.

I don't agree with it being beyond the scope. It's bad for a PR to break tests, as part of the point of tests is so that when different people work on a project they don't accidentally break other parts of the project.

AdamISZ · 2020-09-04T14:31:19Z

I don't agree with it being beyond the scope. It's bad for a PR to break tests, as part of the point of tests is so that when different people work on a project they don't accidentally break other parts of the project.

I think the PR doesn't break tests, as it changes the default setting of native to true: https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/656/files#diff-4f3c2beacd536175f30616463ddaeb33R192 .

And having the test suite pass whether the setting is true or false was not achieved previous to the PR.

openoms · 2020-11-08T22:10:16Z

I have an error when running the yield generator since the last rebase. Seems like some nasty loop repeating:

[MainThread  ] [INFO ]  Warning: too high txfee to be profitable, halfing it to: 0

Settings are as per: #632 (comment) (was working previously)

running the YG: J5AV2ohpe4A91NvU.log: https://termbin.com/sfwr

joinmarket.cfg: https://termbin.com/2vcs

yg-privacyenhanced.py is on the defaults (with sw0reloffer): https://termbin.com/hnk0o
The wallet used is native segwit with tb1... addresses.

EDIT tested @ this commit:
9ed75c2 (HEAD -> pr656) Taker chooses nversion, nlocktime per wallet type

AdamISZ · 2020-11-08T22:23:09Z

Test suite is passing as of 98e838c but note, as per comments earlier in the thread, native in POLICY must be set to true (which is the default) for tests to pass.

While this change was trivial, there is something interesting behind it: the Taker object's filter_orderbook function depends on the setting of native in the config object, which is why native must be set correctly for coinjoins, not only for wallet creation; this can be changed similarly to how we just changed the offer setting for Makers. But this can be done later, it's not critical.

AdamISZ · 2020-11-08T22:24:23Z

I have an error when running the yield generator since the last rebase. ...

thanks for the report; could you give an exact commit, since there were several small changes today?

docs/YIELDGENERATOR.md

AdamISZ · 2020-11-08T22:32:11Z

yg-privacyenhanced.py is on the defaults (with sw0reloffer): https://termbin.com/hnk0o

If you're running on latest commit, this is no longer correct, see last sentence of #656 (comment)

AdamISZ · 2020-11-08T22:36:36Z

@openoms as well as the other questions above, also: is the error condition repeatable?

jmclient/jmclient/yieldgenerator.py

kristapsk · 2020-11-08T23:02:45Z

Test suite now passes for me too.

openoms · 2020-11-09T01:57:35Z

[MainThread  ] [INFO ]  Warning: too high txfee to be profitable, halfing it to: 0

Updated to the latest commit in this PR (d21f30 update Yieldgen docs for offer type) and this issue still persists.
Set ordertype = 'reloffer' in yg-privacyenhanced.py`.

With absoffer it starts fine:

[INFO]  offerlist=[{'oid': 0, 'ordertype': 'sw0absoffer', 'minsize': 91059, 'maxsize': 23215043, 'txfee': 85, 'cjfee': '0.000000'}]

Also the ob-watcher.py is stuck at:

[WARNING]  matplotlib not found, charts will not be available. Do `pip install matplotlib` in the joinmarket virtualenv.              │
[INFO]  Starting ob-watcher

might be related?

AdamISZ · 2020-11-09T10:26:57Z

@openoms reproduced. looking at it today.

AdamISZ · 2020-11-09T11:12:08Z

@openoms should be fixed after 212b2f0

openoms · 2020-11-09T11:39:09Z

@openoms should be fixed after 212b2f0

Tested and reloffer is working now:

[INFO ]  offerlist=[{'oid': 0, 'ordertype': 'sw0reloffer', 'minsize': 102112, 'maxsize': 22445717, 'txfee': 1, 'cjfee': '0.000021'}]

The ob-watcher.py is still stalling though.

AdamISZ · 2020-11-09T12:44:31Z

@openoms so far cannot reproduce, but I tested on regtest with this:

(jmvenv) waxwing@here~/testjminstall/joinmarket-clientserver/scripts/obwatch$ python ob-watcher.py -p 8101 --datadir=../.
User data location: ../.
2020-11-09 12:41:34,400 [DEBUG]  rpc: getblockchaininfo []
2020-11-09 12:41:34,402 [DEBUG]  rpc: getnewaddress []
2020-11-09 12:41:34,426 [INFO]  Starting ob-watcher
2020-11-09 12:41:34,427 [DEBUG]  building irc
2020-11-09 12:41:34,428 [DEBUG]  build_irc:  localhost 6667 #joinmarket-pit-test
2020-11-09 12:41:34,429 [DEBUG]  building irc
2020-11-09 12:41:34,429 [DEBUG]  build_irc:  localhost 6668 #joinmarket-pit-test
2020-11-09 12:41:35,745 [DEBUG]  signedOn:  waxwing-N85-N87-HJ-HJ1-HK1
2020-11-09 12:41:35,747 [DEBUG]  signedOn:  waxwing-N85-N87-HJ-HJ1-HK1
2020-11-09 12:41:37,047 [INFO]  joined: #joinmarket-pit-test waxwing-N85-N87-HJ-HJ1-HK1
2020-11-09 12:41:37,048 [DEBUG]  topicUpdated:  waxwing-N85-N87-HJ-HJ1-HK1 #joinmarket-pit-test 
2020-11-09 12:41:37,049 [INFO]  joined: #joinmarket-pit-test waxwing-N85-N87-HJ-HJ1-HK1
2020-11-09 12:41:37,049 [DEBUG]  topicUpdated:  waxwing-N85-N87-HJ-HJ1-HK1 #joinmarket-pit-test 
2020-11-09 12:41:37,050 [INFO]  Could not connect to *ALL* servers yet, waiting up to 60 more seconds.
2020-11-09 12:41:37,051 [INFO]  All IRC servers connected, starting execution.
2020-11-09 12:41:37,052 [DEBUG]  >>pubmsg !orderbook
2020-11-09 12:41:37,052 [DEBUG]  >>pubmsg !orderbook

started http server, visit http://localhost:8101/

AdamISZ · 2020-11-09T12:57:53Z

@openoms i also tested on mainnet, seems to work fine. not sure what's happening with you there.

AdamISZ · 2020-11-09T15:10:09Z

I think we are ready for squash (of my additions, @Jules23 commits can stay as is imo) and merge (ob-watcher problem above is due to blocked testnet IRC channel, so not relevant here).

Except, we need to update references in the documentation. This is not critical as long as the basic instructions are all correct for the default wallet type; so I'm not inclined e.g. to change every example screenshot for the GUI, yet.
But I will edit all the main documentation for users: README.md, USAGE.md, etc.

and I will delete the old files:

SEGWIT-UPGRADE.md
wallet-upgrade.md

... because they both refer to years-old upgrades and will cause a bunch of confusion to current users, if they read them.

While I'm doing this if anyone wants to sanity check by running a bot or two on mainnet that would be great :)

openoms · 2020-11-09T15:29:56Z

IRC server is fixed. My testnet offer is ready for testing.
Order book: http://t4yapmtli72jnmdvntazv3qv5vm22uiwg7olzksmbbhbqqwwmto7p7yd.onion/

AdamISZ · 2020-11-09T16:05:30Z

b1151d3 is I believe a complete update of the docs to account for the new default wallet type (plus a small number of very minor unrelated updates as I was reading through it). Please let me know if I missed something.

kristapsk · 2020-11-10T00:42:35Z

Not sure it's related with this PR, but tried doing CJ as a taker in testnet and it kinda hanged up.

2020-11-10 02:15:54,902 [INFO]  All IRC servers connected, starting execution.
2020-11-10 02:15:54,906 [DEBUG]  Pubmsging: !orderbook
2020-11-10 02:15:54,906 [DEBUG]  >>pubmsg !orderbook
2020-11-10 02:15:54,906 [DEBUG]  >>pubmsg !orderbook
2020-11-10 02:15:54,907 [INFO]  JM daemon setup complete
2020-11-10 02:16:54+0200 [JMDaemonServerProtocol,0,127.0.0.1] About to send orderbook of size: 7
2020-11-10 02:16:54,909 [INFO]  INFO:Received offers from joinmarket pit
2020-11-10 02:16:54,909 [DEBUG]  considered orders = 
({'counterparty': 'J551JYEJfHQ3hfKj', 'oid': 0, 'ordertype': 'sw0reloffer', 'minsize': 107313, 'maxsize': 24480897, 'txfee': 2, 'cjfee': '0.000020'}, 2)
2020-11-10 02:16:54,909 [DEBUG]  chosen orders = 
{'counterparty': 'J551JYEJfHQ3hfKj', 'oid': 0, 'ordertype': 'sw0reloffer', 'minsize': 107313, 'maxsize': 24480897, 'txfee': 2, 'cjfee': '0.000020'}
2020-11-10 02:16:54,909 [INFO]  Chose these orders: {'J551JYEJfHQ3hfKj': {'cjfee': '0.000020',
                      'counterparty': 'J551JYEJfHQ3hfKj',
                      'maxsize': 24480897,
                      'minsize': 107313,
                      'oid': 0,
                      'ordertype': 'sw0reloffer',
                      'txfee': 2}}
2020-11-10 02:16:54,910 [INFO]  total cj fee = 2
2020-11-10 02:16:54,910 [INFO]  total coinjoin fee = 0.001%

Then, when I pressed Ctrl+C:


2020-11-10 02:39:59+0200 [JMTakerClientProtocol,client] Unhandled Error
        Traceback (most recent call last):
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/protocols/amp.py", line 1041, in _commandReceived
            deferred = self.dispatchCommand(box)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/protocols/amp.py", line 1099, in dispatchCommand
            return maybeDeferred(responder, box)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/internet/defer.py", line 151, in maybeDeferred
            result = f(*args, **kw)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/protocols/amp.py", line 1186, in doit
            return maybeDeferred(aCallable, **kw).addCallback(
        --- <exception caught here> ---
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/internet/defer.py", line 654, in _runCallbacks
            current.result = callback(current.result, *args, **kw)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/protocols/amp.py", line 1169, in checkKnownErrors
            key = error.trap(*command.allErrors)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/python/failure.py", line 460, in trap
            self.raiseException()
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/python/failure.py", line 488, in raiseException
            raise self.value.with_traceback(self.tb)
          File "/home/neonz/git5/joinmarket-clientserver/jmvenv/lib/python3.6/site-packages/twisted/internet/defer.py", line 151, in maybeDeferred
            result = f(*args, **kw)
          File "/mnt/kaka/home/neonz/git5/joinmarket-clientserver/jmclient/jmclient/client_protocol.py", line 456, in on_JM_OFFERS
            retval = self.client.initialize(self.orderbook)
          File "/mnt/kaka/home/neonz/git5/joinmarket-clientserver/jmclient/jmclient/taker.py", line 212, in initialize
            if not self.filter_orderbook(orderbook, sweep):
          File "/mnt/kaka/home/neonz/git5/joinmarket-clientserver/jmclient/jmclient/taker.py", line 271, in filter_orderbook
            self.cjamount)
          File "./scripts/sendpayment.py", line 233, in filter_orders_callback
            if input('send with these orders? (y/n):')[0] != 'y':
        builtins.IndexError: string index out of range

2020-11-10 02:39:59,585 [WARNING]  Connection had broken pipe, attempting reconnect.
^C2020-11-10 02:39:59+0200 [-] Received SIGINT, shutting down.
2020-11-10 02:39:59,904 [INFO]  STALL MONITOR:
2020-11-10 02:39:59,905 [INFO]  Stall detected. Regenerating transactions and retrying.
2020-11-10 02:39:59,906 [INFO]  A transaction failed but there are insufficient honest respondants to continue; giving up.

kristapsk · 2020-11-10T00:46:36Z

Did the same testnet test with JoinMarketQt, it succeeded.

AdamISZ · 2020-11-10T09:06:51Z

@kristapsk on regtest, there's a weird/annoying bug where the prompt 'do you want to send?' doesn't show up, but it's still waiting for you to type 'y' or 'n'. I can't remember if it also applies to testnet, but I think it does (I can check; if you're confused, it's something to do with twisted logging, which is switched off for mainnet).

Either way, we don't get this on mainnet.

AdamISZ · 2020-11-10T09:53:30Z

@kristapsk i can confirm that i can reproduce (no 'builtinserror' on ctrl-c but basically) and stop the error by removing this:

joinmarket-clientserver/scripts/sendpayment.py

Lines 314 to 315 in b1151d3

    
           if jm_single().config.get("BLOCKCHAIN", "network") in ["regtest", "testnet"]: 
        
               startLogging(sys.stdout)

Pretty sure this is not in any way related to the PR

kristapsk · 2020-11-10T10:02:35Z

@AdamISZ Can confirm, removing these lines solves the issue. Do we need that twisted logging on testnet? Maybe can be removed?

AdamISZ · 2020-11-10T11:34:16Z

Yeah, can be removed. I can add it for certain tests if i need it.

* yg scripts set reloffer/absoffer only: Prior to this commit, the yield generator user level scripts required the user to specify offer types depending on the wallet, but only 'rel/abs' distinction is user choice; the other element (native segwit, p2sh or p2pkh) must be defined by the wallet, so we now call `wallet.get_txtype()` to translate from reloffer/absoffer to sw0.. etc. * Taker chooses nversion, nlocktime per wallet type: Takers who are still using p2sh-p2wpkh wallets will not want to flag their transactions with different tx metadata than previous versions that are still running, so we check the `get_txtype()` output to decide which nVersion and nLockTime to use. Also, the SNICKER locktime is reverted to zero as according to draft spec. * change offer type in test_coinjoin * update docs for bech32 wallets

AdamISZ · 2020-11-10T15:32:44Z

Squashed my changes down to one commit, merged to master. Feel free to publicise that widely. If you do that, also make sure to let people know about 'reloffer' in the script settings, as well as the obvious one about wallet type.

decentclock · 2020-11-10T17:18:31Z

Great! Thank you so much @AdamISZ @kristapsk @openoms for getting this over the finish line!

MaxHillebrand · 2020-11-10T18:11:11Z

This is wonderful - fantastic work - thanks a lot to all devs & reviewers - eager to see how the bootstrapping of this new orderbook will go...

Since JoinMarket-Org/joinmarket-clientserver#656 JoinMarket supports bech32 addresses also for CoinJoins, with a dedicated orderbook.

RiccardoMasutti suggested changes Aug 4, 2020

View reviewed changes

jmclient/jmclient/configure.py Outdated Show resolved Hide resolved

jmclient/jmclient/configure.py Outdated Show resolved Hide resolved

MaxHillebrand reviewed Aug 4, 2020

View reviewed changes

AdamISZ reviewed Aug 5, 2020

View reviewed changes

jmclient/jmclient/taker.py Show resolved Hide resolved

AdamISZ reviewed Aug 5, 2020

View reviewed changes

decentclock mentioned this pull request Aug 6, 2020

Docs: add commitments explainer #662

Merged

decentclock marked this pull request as draft August 7, 2020 01:03

decentclock mentioned this pull request Aug 7, 2020

Drop key_type argument to import_private_key #661

Merged

decentclock marked this pull request as ready for review August 8, 2020 15:32

decentclock marked this pull request as draft August 8, 2020 16:03

decentclock marked this pull request as ready for review August 8, 2020 19:11

AdamISZ reviewed Nov 8, 2020

View reviewed changes

docs/YIELDGENERATOR.md Outdated Show resolved Hide resolved

kristapsk reviewed Nov 8, 2020

View reviewed changes

jmclient/jmclient/yieldgenerator.py Show resolved Hide resolved

AdamISZ merged commit be8bfc4 into JoinMarket-Org:master Nov 10, 2020

kristapsk mentioned this pull request Nov 11, 2020

Remove twisted logging on testnet #726

Merged

PulpCattel added a commit to PulpCattel/Observatory that referenced this pull request Nov 16, 2020

Add bech32 to JoinMarket criteria

c2c8ba9

Since JoinMarket-Org/joinmarket-clientserver#656 JoinMarket supports bech32 addresses also for CoinJoins, with a dedicated orderbook.

Bech32 orderbook #656

Bech32 orderbook #656

Conversation

decentclock commented Aug 4, 2020 • edited Loading

RiccardoMasutti left a comment • edited Loading

Choose a reason for hiding this comment

MaxHillebrand left a comment

Choose a reason for hiding this comment

decentclock commented Aug 4, 2020

decentclock commented Aug 4, 2020 • edited Loading

AdamISZ commented Aug 5, 2020

dgpv commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamISZ Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentclock Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamISZ Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamISZ commented Aug 5, 2020 • edited Loading

AdamISZ Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

AdamISZ commented Aug 5, 2020

AdamISZ commented Aug 5, 2020

AdamISZ commented Aug 5, 2020

decentclock commented Aug 8, 2020 • edited Loading

decentclock commented Aug 8, 2020

AdamISZ commented Aug 16, 2020

decentclock commented Aug 19, 2020 • edited Loading

kristapsk commented Aug 19, 2020

decentclock commented Aug 19, 2020 • edited Loading

decentclock commented Sep 4, 2020

chris-belcher commented Sep 4, 2020

AdamISZ commented Sep 4, 2020

openoms commented Nov 8, 2020 • edited Loading

AdamISZ commented Nov 8, 2020

AdamISZ commented Nov 8, 2020

AdamISZ commented Nov 8, 2020

AdamISZ commented Nov 8, 2020

kristapsk commented Nov 8, 2020

openoms commented Nov 9, 2020 • edited Loading

AdamISZ commented Nov 9, 2020

AdamISZ commented Nov 9, 2020

openoms commented Nov 9, 2020

AdamISZ commented Nov 9, 2020

AdamISZ commented Nov 9, 2020

AdamISZ commented Nov 9, 2020

openoms commented Nov 9, 2020 • edited Loading

AdamISZ commented Nov 9, 2020

kristapsk commented Nov 10, 2020

kristapsk commented Nov 10, 2020

AdamISZ commented Nov 10, 2020

AdamISZ commented Nov 10, 2020

kristapsk commented Nov 10, 2020

AdamISZ commented Nov 10, 2020

AdamISZ commented Nov 10, 2020

decentclock commented Nov 10, 2020

MaxHillebrand commented Nov 10, 2020

decentclock commented Aug 4, 2020 •

edited

Loading

RiccardoMasutti left a comment •

edited

Loading

decentclock commented Aug 4, 2020 •

edited

Loading

AdamISZ Aug 5, 2020 •

edited

Loading

decentclock Aug 5, 2020 •

edited

Loading

AdamISZ Aug 5, 2020 •

edited

Loading

AdamISZ commented Aug 5, 2020 •

edited

Loading

AdamISZ Aug 5, 2020 •

edited

Loading

decentclock commented Aug 8, 2020 •

edited

Loading

decentclock commented Aug 19, 2020 •

edited

Loading

decentclock commented Aug 19, 2020 •

edited

Loading

openoms commented Nov 8, 2020 •

edited

Loading

openoms commented Nov 9, 2020 •

edited

Loading

openoms commented Nov 9, 2020 •

edited

Loading